fix(windowing/input): harden window targeting and pointer reporting - #96
Conversation
![revuto-review[bot]](https://avatars.githubusercontent.com/u/55848801?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is an auto review done by revuto.
Revuto completed the review and found no evidence-backed concerns.
avifenesh
left a comment
avifenesh
left a comment
There was a problem hiding this comment.
Two fixes needed before merge. Pointer axis maxima/clamp look correct and tested.
- P1 — New Registry
Peer.Ping/gdbus introspectcalls use unboundedcmd.output(). GetAddress can succeed on the session bus while the a11y socket is stale, sodoctor/setup_accessibilitycan hang instead of returning a blocker. Wrap both probes in the existing 2scommand_runner::output_blocking_with_timeout. - P2 — When that Registry check fails, readiness still says AT-SPI is disabled and recommends
setup_accessibility/IsEnabled. Surfaceaccessibility.at_spi_bus.detailand do not recommend toolkit enable when the bus was discovered but Registry is unreachable.
Optional (not blocking): exclusive-edge clicks now land on the last pixel while off_screen_note_for_point still warns that the input landed on no visible pixel. Align that warning with the new clamp.
nisavid
changed the title
There was a problem hiding this comment.
This is an auto review done by revuto.
Revuto completed the review and found no evidence-backed concerns.
|
@avifenesh I removed both Registry-probe findings by dropping that out-of-range experiment entirely. The current branch retains only the bounded source-range exports, and the two old review threads are resolved with exact-head evidence. The current immutable head is |
avifenesh
left a comment
avifenesh
left a comment
There was a problem hiding this comment.
Thanks for the fast turnaround, and for tracing the two findings back to source range instead of patching around them. That's the right instinct and it made this easy to verify.
I re-ran the full CI set verbatim against bebbdcc4 in a clean worktree — fmt, check, clippy, 262 + 7 + 1 tests, installer regressions, rustdoc, publish --dry-run, audit, MCP safety (18 tools / 0.4.9), agnix, npm wrapper, zod schema. All green locally, matching hosted. doctor from the PR-head binary on GNOME 50.1 / Wayland still advertises at_spi, reports no new blockers, degrades cleanly.
Both blocking points are resolved. Confirmed by reading the diff, not just the reply: src/diagnostics.rs at head has no Registry reachability probe, and the only surviving accessibility change is src/diagnostics.rs:353 routing capability advertisement through the existing can_build_accessibility_tree. P2 has no new failure state left to reconcile. Threads closed.
Three things before I can approve.
1 — Drop the Niri backend. Policy call on my side, not a code-quality one. niri is GPL-3.0-or-later (workspace Cargo.toml, inherited by niri-ipc on every published version) and this project is MIT. That's the incompatible direction, and it's a hard line for me regardless of how the integration is written.
I want to be clear that I see what you did here. You deliberately avoided the niri-ipc crate, kept it to a runtime niri msg exec, and left Cargo.toml and Cargo.lock genuinely untouched — I checked, and that's accurate. I'll also grant that exec across a process boundary is not linking, so you can make a decent argument that nothing attaches. The line I'm still not crossing is that the local NiriWindow / NiriWindowLayout projection is derived from reading niri-ipc/src/lib.rs. No dependency, no verbatim copy — but the shape came from GPL source, and I'd rather not carry that provenance at all than have to argue about where the boundary sits later, in an MIT tree other people build on.
For context on why I'm rigid about this exact case: wanting IPC-level desktop control without depending on niri is precisely why I built agent-workspace-linux. It gives an agent a hidden, agent-owned desktop and browser over MCP — full window/input/browser control, real compositor semantics, IPC the whole way down — and none of it routes through a copyleft compositor. MIT, so it composes cleanly downstream. It also solves something computer-use-linux structurally can't: the agent gets absolute control of a machine without seizing control of your machine. No focus fights, no stray clicks landing in your editor, no screenshots of your real desktop.
If the Niri work came from wanting a better compositor target to drive, that's where I'd point you — and I mean that as an actual invitation, not a deflection. Good contributors are rare and I'd rather keep working with you than win an argument. There's real work over there I'd hand you tomorrow.
Reverting 45fbafb and bebbdcc should be clean; nothing else here depends on them, so the pointer, KWin, GNOME-extension, and AT-SPI corrections stand on their own. That also removes the README compositor row and the CHANGELOG Added block, which resolves the validation question I'd otherwise have raised — the backend has no real-niri coverage, only JSON fixtures.
2 — Same class as P1, new instance. gnome_extension_enabled() (src/gnome_extension.rs:271-283) calls gsettings get org.gnome.shell enabled-extensions through raw command.output(), reached from setup_window_targeting_report() at line 41. A stalled dconf or session bus hangs setup_window_targeting with no deadline. I know that file already has three unbounded .output() calls, so this matches local style — but it's the exact pattern we just bounded, in a PR titled "bounded corrections." Route it through command_runner::output_blocking_with_timeout with the 2s COMMAND_TIMEOUT. Bounding the three pre-existing ones too would be welcome; I won't hold the PR on those.
3 — The optional item, now nearly free. clamp_coordinates (src/abs_pointer.rs:42) clamps to width-1, so x == width lands on the last visible pixel, while off_screen_note_for_point (src/server.rs:2830) still tests x >= mx+mw and warns "the input landed on no visible pixel." This PR introduced that disagreement; one-line fix while you're in there.
One process note, not a complaint: my review was against da3b3d4 — 3 files, +233/−19. Head is +1112/−261 across 11 files on top of that, including the KWin callback-authentication rework I haven't looked at yet. So this isn't a refresh, it's a first review of new work. Naming it so neither of us mistakes a re-request for a rubber stamp. What I've read of the KWin side looks right: the org.kde.KWin owner check plus method-kind, envelope, and one-shot CAS gating in accept() is the correct shape, and the collision path properly leaves the incumbent registration alone. Caveat that it's all validated against the fake-scripting-on-private-bus harness — nothing here has touched real KWin, and I have no Plasma session to check on either.
Sort out 1–3 and I'll do a full pass on the remaining scope. Appreciate the work.
nisavid
changed the title
|
@avifenesh Thanks for the careful pass and for the pointer to The Niri backend wasn’t motivated by my own setup—I don’t use Niri. It entered the downstream tree through ilysenko/codex-desktop-linux#917, and I carried it over while auditing the downstream Computer Use changes. I treated it as a generic interoperability change and missed your stricter license-provenance policy. I’m happy to remove it rather than leave that boundary for this repository to own.
I’ll address item 2 systematically: route the new You’re right that the expanded patch deserves a complete first review, not a quick recheck. I have a real Plasma/KWin Wayland session available, so before I ask you to look again I’ll test the revised commit there: native-Wayland and Xwayland window listing and activation, minimized and off-desktop windows, multiple monitors, stale-target recovery, repeated and concurrent transactions, cleanup, and coordinate boundaries. I’ll include the environment and results with the revision. I’ll follow up once the changes, tests, and live KWin pass are complete. |
nisavid
changed the title
There was a problem hiding this comment.
This is an auto review done by revuto.
Revuto completed the review and found no evidence-backed concerns.
|
@avifenesh I’ve pushed the implementation revision ( Niri and its registry, diagnostics, environment, test, and documentation wiring are gone. All four GNOME extension subprocess paths now use the shared two-second blocking runner; timeout coverage verifies process-group cleanup, including descendants holding output pipes open. The pointer result is now a The relevant local gates passed: formatting, compilation and linting, targeted behavioral tests, installer regressions, and the 18-tool MCP contract. This sandbox still trips two library tests that also fail on I still owe the Plasma/KWin run I described above. I’ll follow up once I’ve had a chance to complete it, with the environment, results, and any resulting fixes, before asking for a full review. |
There was a problem hiding this comment.
This is an auto review done by revuto.
Revuto completed the review and found no evidence-backed concerns.
|
@avifenesh I completed the Plasma/KWin pass and updated the PR description with the environment, commands, and detailed results. On Plasma/KWin 6.7.4 Wayland, native Wayland and Xwayland targets behaved correctly across normal, minimized, fullscreen, second-monitor, and separate-desktop cases. A stale target was rejected in 46.4 ms, and 50 cycles across five concurrent MCP processes completed without crossed responses or leftover scripts. The run caught one additional Plasma 6 compatibility issue: KWin no longer exposes the legacy I also replaced the original subjective pointer check with two fullscreen Wayland observers. They confirmed both sides of the mixed-scale monitor seam, including the seam coordinate at the second monitor’s first pixel. A request at the combined desktop’s exclusive bottom-right boundary was clamped to the last visible pixel and returned the clamp-specific result instead of the misleading off-screen warning. All test windows, the temporary desktop, and KWin scripts were cleaned up. All hosted checks pass on commit The revised scope is ready for the full pass you mentioned. When you have a chance, I’d appreciate another look. |
|
Thanks @nisavid — went through the revision and the Plasma/KWin results. On your updates:
What I still need before the full line-by-line review:
1 and 2 can be short — environment line + observed results, same style as your KWin writeup. |
|
@avifenesh Thanks for laying out the remaining matrix so clearly. I've run both passes now and put the full environment, timings, receipts, and cleanup notes in the PR description rather than make you dig through another long comment here. The short version: GNOME 50.4 on Wayland focused the intended window in the normal, minimized, second-monitor, and fullscreen cases, then rejected a stale ID in 14.1 ms without disturbing focus. With the extension absent, the portal still failed closed off-screen; ydotool kept the warning, along with its existing acceleration-sensitive landing caveat. The Fedora 44 guest has no genuine GNOME X11 session, so I left that optional variant out. I refreshed against current main as well. The branch is unchanged and nothing else is queued—this is the final shape, ready for the full pass you mentioned. |
avifenesh
left a comment
avifenesh
left a comment
There was a problem hiding this comment.
Approving. Ran the gate set verbatim against 39efa80 in a clean worktree first — I wasn't going to sign off on the KWin rework from the diff alone.
fmt clean. clippy surfaces two nonminimal_bool hits at src/diagnostics.rs:466 and src/server.rs:3615, both in code you never touched and both reproducing on main under a newer local toolchain than CI pins — mine to clean up, not yours. Tests: 255 pass serialized, and the two process-group reaping failures are the sandbox; main fails four of that same family here, so this branch is strictly better than what it lands on. Hosted CI green across all 12 on the head.
On the three items:
1 — Niri. Gone cleanly, and nothing else leaned on it. Thanks for taking the license point at face value rather than arguing where the boundary sits. The agent-workspace-linux offer stands whenever you want it, and I hear you on Wayland — that's a fair reason to pass.
2 — Bounded commands. You did the systematic version instead of the minimum I asked for: all four subprocess paths on the shared 2s runner, with timeout coverage proving process-group cleanup including descendants holding output pipes open. That's the version I wanted and hadn't asked for.
3 — Clamp vs off-screen warning. Checked this rather than took it. capture_space_rect() returns origin (0,0) with the same dimensions AbsPointer::create is handed, so the clamp note and the off-screen warning are exactly complementary on the absolute-pointer path — nothing was lost by moving off_screen_note_for_point down to the fallback branch. The PointerLanding requested/emitted pair is the right contract, and moving the button check ahead of ensure_abs_pointer means an unsupported button falls through without paying for device creation.
On the KWin work, the part I hadn't reviewed before: owner check, method kind, envelope, and the one-shot CAS in accept() close the callback off properly, and both generated scripts stamp backend/pluginName, so a well-formed foreign payload can't starve the channel. Gating cleanup on owns_callback is what makes the collision path safe — the duplicate-path test asserting the incumbent's registration survives with zero load/start/unload calls is the one I'd have written. Plasma 6 classification via serialize(window) object-class parsing is stringly, but it's guarded and it's what KWin leaves you now the legacy flags are gone. Good catch finding that on your own run rather than shipping it broken.
The GNOME and extension-absent passes close what I asked for. GNOME-on-X11 being unavailable in your guest is fine — flagging it not-run instead of counting Xwayland is the honest call, and the xdotool spawn-failure regression covers the routing anyway.
Squashing on merge; the branch carries the Niri add-and-remove round trip and there's no reason for main to.
Appreciate the work on this one — tracing findings back to the source range instead of patching around them, standing up a real Plasma session to validate, and re-running a matrix rather than asserting it would pass. This is the shape of contribution I want more of here. Thank you.
Generated by Claude Code
Roll the unreleased windowing and input hardening from #96 into 0.4.10 and bump the Cargo, lockfile, npm, and advertised MCP server versions to match. Also carries two lint fixes: replace the double-negated WAYLAND_DISPLAY emptiness checks with is_none_or, and read COSMIC toplevel state words through as_chunks so the -D warnings build passes on Rust 1.98, which added chunks_exact_to_as_chunks. Neither changes behavior.
src/abs_pointer.rssrc/cli.rssrc/command_runner.rssrc/diagnostics.rssrc/gnome_extension.rssrc/server.rssrc/windowing/backends/kwin.rssrc/abs_pointer.rssrc/cli.rssrc/command_runner.rssrc/diagnostics.rssrc/gnome_extension.rssrc/server.rssrc/windowing/backends/kwin.rssrc/windowing/mod.rsCHANGELOG.mdsrc/windowing/backends/kwin.rsSummary
Provenance and scope
This PR began as the standalone export of generic Computer Use changes in two bounded downstream ranges:
26db7074..781201d798f2338d..19239c06The audited downstream Computer Use tree is
0f122b7cdeb26bf13faacaf8b16445ee18a7d62e, retained by downstream audit commit4762b207. The current PR comparison is663930fef8d1bee64b7b8f833b68cff3196059e3..39efa804b32c6f443a0c1d2230579c482ceb599f.Review and manual validation added narrow corrections on the same affected surfaces: bounded GNOME setup commands, emitted pointer reporting, and Plasma 6 client classification. Downstream app authorization, identity, browser-host, cursor side-channel, package, and numeric-version contracts remain out of scope.
Review path
src/command_runner.rsandsrc/gnome_extension.rsfor the bounded setup-command path and extension reload contract.src/abs_pointer.rs,src/server.rs, andsrc/cli.rsfor the pointer landing contract and fallback behavior.src/windowing/backends/kwin.rs: owner resolution, callback envelope, one-shot delivery, transaction timeout, collision-safe cleanup, Plasma 6 client classification, then the private-bus adversarial tests.src/diagnostics.rs.Author-run operational evidence
These completed scenarios summarize the evidence already gathered; they are not a request to repeat the full matrix.
client_type: waylandand Xmessage reportedclient_type: x11.(3440,720)landed at local x=0 on the borrowed monitor.(5744,1440)emitted as(5743,1439)through MCP andabs-test; negative and oversized inputs also clamped with no generic warning.640×440, and receipts wereA, B, B, A.No window matched window_id 3718609245.returned in 14.1 ms with no focus result, while A stayed focused.xrandraddition,(500,400)produced one B receipt at(483,395);(2048,100)returnedok: falsewith the generic warning, no receipt, and no ydotool replay.(200,200)returnedok: trueand produced one fullscreen receipt.(2048,100)kept the warning but acceleration distorted the landing onto the visible target's left edge, which is why absolute uinput remains preferred.How it was run
cargo build --locked,target/debug/computer-use-linux doctor, andtarget/debug/computer-use-linux windows, then drovelist_windows,activate_window, andfocused_windowthrough a direct MCP harness without persistent configuration.doctorfound KWin, AT-SPI, focus, and development input ready. Cleanup: closed both window sets, removed the desktop, and found no script or journal residue./dev/uinputand harmless fullscreen targets on both displays, I clicked around the seam and rantarget/debug/computer-use-linux abs-test 5744 1440. Cleanup: closed both targets and found no remaining window or KWin script.1024×768heads, nonce-labelled GTK windows supplied compositor IDs and click receipts while list, focus, move, and activation went through MCP. Cleanup: closed both targets, confirmed a fresh list contained neither target, and terminated every MCP server; the guest and its volumes remain available for review follow-up.xrandraddition and portal consent remain in the retained guest.Revision binding: KWin, pointer, and stress coverage exercised
6c5a7c9; Plasma 6 classification in2d7ab1fwas retested on native Wayland and Xwayland;39efa80only records the result. The GNOME checkout had no product-path difference from39efa804, and the installed extension byte-matched it.Automated verification
cargo fmt --all -- --check,cargo check --locked --all-targets, andcargo clippy --locked --all-targets -- -D warningspassed.cargo test --locked windowing:: -- --nocapturepassed all 56 windowing tests.cargo test --locked --no-fail-fast -- --test-threads=1passed 255 of 257 library tests; both failures are unchanged ydotool harness tests. COSMIC's 7 tests and the library-export test passed. The focusedfakeroot cargo test --locked ydotool::tests::accepts_supported_raw_cli_without_xdg_runtime_dir -- --exact --nocapturepassed; the continuous-output timing test still exceeds its one-second sandbox threshold.bash -n install.sh scripts/install_sh_test.shandscripts/install_sh_test.shpassed all 12 cases.cargo build --locked,RUSTDOCFLAGS="-D warnings" cargo doc --locked --no-deps --document-private-items, andcargo publish --dry-run --lockedpassed.scripts/mcp_safety_check.py --binary target/debug/computer-use-linuxpassed for all 18 tools at version 0.4.9.agnix, the clean-host suite, and Revuto; release and publication jobs were skipped as expected.Remaining
The published branch is at its final shape:
upstream/mainremains663930fef8d1bee64b7b8f833b68cff3196059e3, the remote PR head remains39efa804b32c6f443a0c1d2230579c482ceb599f, and the base is an ancestor with no commits to reconcile. Live validation produced no product changes, and nothing else is queued. This revision is ready for the maintainer's full review.